Support single/split deployments, optimize image processing, and expand test coverage - #7
Merged
Vivek-M-08 merged 10 commits intoJul 31, 2026
Conversation
… advisory lock run_all.sh's single-container mode starts web/consumer/worker as three separate OS processes, each independently calling db.connect()/initialize_schema() at startup. The existing asyncio.Lock only serializes coroutines within one process, so on a cold database the three processes raced on the same schema DDL and one crashed with a Postgres catalog collision (duplicate key on pg_type), killing the whole container. Discovered while load-testing the single-container profile. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…fective default Load-testing 200 concurrent real-time submissions against the hardcoded min_size=2/max_size=10 pool caused nearly the entire batch (196/200) to stall indefinitely in "processing" — activities queued for a DB connection and were cancelled rather than failing fast, since the pool was drastically undersized for that concurrency. Exposed as DATABASE_POOL_MIN_SIZE/DATABASE_POOL_MAX_SIZE settings so it's tunable per environment without a code change; re-running the same 200-event batch with the pool raised to 5/50 completed 198/200 in 6 minutes (the other 2 failed on an unrelated malformed-LLM-JSON response, not a pool issue). Co-Authored-By: Claude Sonnet 5 <[email protected]>
- Set on Temporal worker. - Replace default asyncio thread pool executor with custom ThreadPoolExecutor sized to worker limits. - Cap PyTorch CPU intra-op threads to 1 to avoid thread thrashing during concurrent embeddings.
- Parallelize image face-blurring with asyncio.gather and semaphore concurrency bounds. - Defer DB connection acquisition in deface_blur_activity to avoid holding idle connections during image processing. - Remove premature submission status updates from pii_and_abusive_activity.
- Parallelize image face-blurring with and semaphore concurrency bounds. - Defer DB connection acquisition in to avoid holding idle connections during image processing. - Remove premature submission status updates from .
- Define WORKER_MAX_CONCURRENT_ACTIVITIES, image executor settings, and update DB pool defaults in app/config.py and .env.example.
WalkthroughThe change adds configurable runtime limits, synchronized resource initialization, concurrent image processing, workflow-owned terminal statuses, active Docker Compose services, and comprehensive mocked unit tests with shared infrastructure and fixtures. ChangesRuntime configuration and deployment
Resource initialization
Image processing and workflow status
Test infrastructure and coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DefaceBlurActivity
participant ImageExecutor
participant GCS
participant Database
DefaceBlurActivity->>Database: Release connection
DefaceBlurActivity->>ImageExecutor: Process images with bounded concurrency
ImageExecutor->>GCS: Download and upload images
DefaceBlurActivity->>Database: Persist ordered results
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
app/temporal/deface_blur_activity.py (2)
38-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the module-global type annotation.
_blur_semaphoreis annotated asasyncio.Semaphorebut initialized toNone. UseOptionalso type checkers accept the sentinel value.♻️ Proposed annotation fix
-_blur_semaphore: asyncio.Semaphore = None -_blur_semaphore_loop = None +_blur_semaphore: Optional[asyncio.Semaphore] = None +_blur_semaphore_loop: Optional[asyncio.AbstractEventLoop] = NoneAdd
Optionalto the typing import:-from typing import Dict, Any +from typing import Any, Dict, Optional🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/temporal/deface_blur_activity.py` around lines 38 - 39, Update the _blur_semaphore module-global annotation to use Optional[asyncio.Semaphore], and add Optional to the existing typing imports so its initial None sentinel is type-correct.
108-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the path returned by
_download_file.Line 108 recomputes
DOWNLOADS_DIR / filename, and_download_filecomputes the same path at line 71. The return value is discarded. If either expression changes, thefinallycleanup deletes the wrong path and leaks the real temporary file.♻️ Proposed fix to keep one source of truth
- local_path = DOWNLOADS_DIR / filename + local_path = DOWNLOADS_DIR / filename # provisional; reassigned from the download result output_path = OUTPUTS_DIR / f"blurred_{filename}" try: # 1. Download file locally - await _run_in_image_executor(_download_file, resolved_url, filename) + local_path = await _run_in_image_executor(_download_file, resolved_url, filename)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/temporal/deface_blur_activity.py` around lines 108 - 113, Update the activity flow around _run_in_image_executor and _download_file to capture the downloader’s returned local path and use it for subsequent processing and finally cleanup. Remove the independently recomputed DOWNLOADS_DIR / filename path, while preserving the existing output_path behavior.tests/conftest.py (2)
265-292: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueThe outcome map is process-global, so parallel runs record incomplete data.
_test_outcomesis a module-level dict. Underpytest-xdist, each worker collects only its own subset. Every worker then runspytest_sessionfinishand rewrites the CSV with its partial view, so the last writer wins.If parallel execution is not planned, no change is needed. If it is planned, restrict the sync to the controller process, for example by checking
session.config.workerinputand returning early in workers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 265 - 292, The module-level _test_outcomes map is incomplete in pytest-xdist workers, causing each worker to overwrite the CSV with partial results. Update pytest_sessionfinish to detect session.config.workerinput and return immediately for worker processes, leaving CSV synchronization to the controller while preserving the existing outcome aggregation.
295-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the CSV rewrite opt-in and fail-safe.
pytest_sessionfinishrewritestests/TEST_CASES.csvon every session. Three concerns follow from that:
- The file is version-controlled, so every local or CI run produces a dirty working tree and unrelated diff noise.
header.index("Test ID")andheader.index("Status")raiseValueErrorif a column is renamed. The exception then surfaces at session finish and masks the real test results.- The write is not atomic. An interruption between
open(..., "w")andwriterowstruncates the file and loses the recorded statuses.Gate the sync behind an environment flag, guard the header lookup, and write through a temporary file.
♻️ Proposed change
def pytest_sessionfinish(session, exitstatus): if not _test_outcomes or not _CSV_PATH.exists(): return + if os.environ.get("SYNC_TEST_CASES_CSV", "").lower() not in ("1", "true", "yes"): + return with open(_CSV_PATH, newline="", encoding="utf-8") as f: rows = list(csv.reader(f)) header = rows[0] - id_idx = header.index("Test ID") - status_idx = header.index("Status") + if "Test ID" not in header or "Status" not in header: + return + id_idx = header.index("Test ID") + status_idx = header.index("Status") @@ - with open(_CSV_PATH, "w", newline="", encoding="utf-8") as f: - writer = csv.writer(f, quoting=csv.QUOTE_MINIMAL, lineterminator="\n") - writer.writerows(rows) + tmp_path = _CSV_PATH.with_suffix(".csv.tmp") + with open(tmp_path, "w", newline="", encoding="utf-8") as f: + writer = csv.writer(f, quoting=csv.QUOTE_MINIMAL, lineterminator="\n") + writer.writerows(rows) + os.replace(tmp_path, _CSV_PATH)Add
import osat the top of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 295 - 325, Update pytest_sessionfinish to run CSV synchronization only when the designated environment flag is enabled, while preserving the existing _test_outcomes and _CSV_PATH checks. Guard the header.index lookups for “Test ID” and “Status” so malformed or renamed columns return without rewriting or masking test results, and replace the direct CSV write with an atomic temporary-file write followed by replacement of _CSV_PATH. Add the required os import.tests/unit_testing.py (4)
755-765: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
_find_execute_callinstead of the lastexecutecall.The
_find_execute_calldocstring at lines 59-65 states that the lastexecutecall is not reliably the one under test, because metadata upserts also run throughconn.execute. This test still indexescall_args_list[-1]. The same pattern appears at lines 1149 and 1167.♻️ Proposed change
- insert_call = conn.execute.call_args_list[-1] + insert_call = _find_execute_call(conn, "INSERT INTO story_submissions") objective_arg = insert_call.args[4]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_testing.py` around lines 755 - 765, Update test_db_002_insert_story_stores_scalar_text and the similar tests around the referenced locations to use the existing _find_execute_call helper instead of indexing conn.execute.call_args_list[-1]. Extract objective_arg from the helper’s targeted call while preserving the current assertion.
1971-1981: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAsserting on source text is brittle.
inspect.getsourcematching breaks on any whitespace or formatting change intry_claim_for_processing, even when the SQL semantics stay identical. The test then fails without a behavior change.Assert on the executed SQL instead. Call
try_claim_for_processingwith aFakeConnand inspect the statement passed toconn.fetchvalorconn.execute. That checks the same single-statement compare-and-swap shape and survives reformatting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_testing.py` around lines 1971 - 1981, The test test_upload_023_concurrent_process_calls_race_safe currently relies on brittle inspect.getsource text matching. Replace it with a FakeConn-based invocation of try_claim_for_processing, capture the SQL passed to fetchval or execute, and assert that the executed statement is a single UPDATE compare-and-swap containing the status predicate and RETURNING status.
431-456: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThree tests each wait 7 real seconds; replace the real backoff wait.
test_kafka_020,test_kafka_021, andtest_kafka_023each call_run_consumer_briefly(..., duration=7.0)to let the real 2s and 4s retry backoff elapse. That adds at least 21 seconds to every run and makes the assertions timing-dependent on machine load.Make the backoff configurable in
app/kafka/consumer.py, for example as a module-level constant or a setting, then override it in these tests. The retry-count and DLQ assertions stay valid and the wall time drops to well under a second.#!/bin/bash # Locate the retry/backoff implementation in the consumer to confirm an override point. fd -H -t f 'consumer.py' -p app -x rg -n -C 5 'sleep|backoff|attempt|MAX_RETRIES|range\(' {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_testing.py` around lines 431 - 456, Make the retry backoff used by IngestionConsumer configurable in app/kafka/consumer.py, using a module-level constant or existing settings symbol at the sleep calculation. In test_kafka_020_db_failure_retries_3_times_then_dlq and the corresponding test_kafka_021 and test_kafka_023 flows, override that value with near-zero delays before invoking _run_consumer_briefly, then reduce the test duration accordingly while preserving the existing retry-count and DLQ assertions.
613-620: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the blind
pytest.raises(Exception)assertions.Ruff reports B017 at lines 614, 633, 1024, 1046, 1257, 1273, and 1660.
pytest.raises(Exception)passes for any failure, including anAttributeErrorfrom a wrong patch target. The test then reports success while the code path under test never ran.Use the specific exception type each code path raises, for example
ValueErrororjson.JSONDecodeError.Also applies to: 1023-1032, 1256-1263
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_testing.py` around lines 613 - 620, Replace each broad pytest.raises(Exception) assertion in the affected tests, including the cases around pii_and_abusive_language_detection_activity, with the specific exception type raised by that code path, such as ValueError or json.JSONDecodeError. Verify each expected type from the implementation and preserve the existing log assertions so patching errors cannot satisfy the tests accidentally.Source: Linters/SAST tools
tests/csv_uploads/not_a_csv.txt (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis fixture is not used by any test.
test_upload_006_non_csv_extension_rejectedintests/unit_testing.pybuilds its payload inline at line 1758 instead of loading this file. Use_csv_file("not_a_csv.txt")there, or remove this fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/csv_uploads/not_a_csv.txt` around lines 1 - 2, Remove the unused tests/csv_uploads/not_a_csv.txt fixture, or update test_upload_006_non_csv_extension_rejected in tests/unit_testing.py to load it via _csv_file("not_a_csv.txt") instead of constructing the payload inline; choose one approach and ensure the test continues validating rejection of a non-CSV extension.docker-compose.yaml (2)
69-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce duplicated environment blocks across the four application services.
analytics-web,analytics-consumer,analytics-worker, andanalytics-allrepeat the identicalenv_file,environment,extra_hosts, anddepends_onblocks. Extract a YAML anchor orx-extension field for the shared configuration so future endpoint changes only need one edit.♻️ Example using a YAML anchor
+x-app-common: &app-common + build: . + image: elevate-analytics:latest + env_file: .env + environment: + DATABASE_URL: postgresql://postgres:[email protected]:5432/analytics_db + TEMPORAL_HOST: temporal:7233 + KAFKA_BOOTSTRAP_SERVERS: kafka:9092 + extra_hosts: + - "host.docker.internal:host-gateway" + depends_on: + temporal: + condition: service_healthy + kafka: + condition: service_healthy + services: analytics-web: + <<: *app-common profiles: ["split"] - build: . - image: elevate-analytics:latest - env_file: .env - environment: - DATABASE_URL: postgresql://postgres:[email protected]:5432/analytics_db - TEMPORAL_HOST: temporal:7233 - KAFKA_BOOTSTRAP_SERVERS: kafka:9092 - extra_hosts: - - "host.docker.internal:host-gateway" - depends_on: - temporal: - condition: service_healthy - kafka: - condition: service_healthy ports: - "8000:8000" volumes: - ./logs:/app/logs command: ["--mode", "web"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yaml` around lines 69 - 154, Extract the identical env_file, environment, extra_hosts, and depends_on configuration into a shared YAML anchor or x- extension field, then reuse it in analytics-web, analytics-consumer, analytics-worker, and analytics-all. Preserve each service’s existing profile, ports, volumes, and command or entrypoint settings while ensuring the shared configuration is defined only once.
69-154: 🧹 Nitpick | 🔵 TrivialConsider adding healthchecks to the application services.
temporal,temporal-ui, andkafkadefine healthchecks, butanalytics-web,analytics-consumer,analytics-worker, andanalytics-alldo not. A healthcheck on these services would letdocker compose psand any orchestration layer detect a stuck or crash-looping process, rather than relying ondepends_on: condition: service_healthyfrom other services alone.As per path instructions for
docker-compose*.yaml: "Validate Docker Compose configuration for: - Proper networking - Secrets handling - Health checks - Restart policies - Volume mappings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yaml` around lines 69 - 154, Add appropriate Docker Compose healthchecks to analytics-web, analytics-consumer, analytics-worker, and analytics-all so each service reports whether its application process is responsive or healthy. Use the existing service-specific commands, endpoints, or modes to define checks that work inside the containers, while preserving their current dependencies, ports, volumes, and startup commands.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/config.py`:
- Around line 16-22: Update the Settings configuration model containing
DATABASE_POOL_MIN_SIZE and DATABASE_POOL_MAX_SIZE to add a model_validator that
rejects configurations where the minimum exceeds the maximum, raising a clear
error that names both environment variables; retain the existing positive-value
validation.
In `@app/temporal/deface_blur_activity.py`:
- Around line 186-195: Update the result validation after asyncio.gather in the
activity’s bounded image-processing flow to check for BaseException rather than
Exception, then immediately re-raise any such result. Preserve normal dictionary
result handling for successful calls so cancellation and heartbeat failures
propagate without reaching the URL list comprehensions.
- Around line 69-77: Validate each resolved image URL in the image-download flow
before `_download_file` calls `urllib.request.urlopen`, allowing only expected
media hosts derived from `settings.MEDIA_BASE_URL` and any explicit media-domain
allowlist. Preserve support for absolute HTTP(S) URLs and paths joined through
`MEDIA_BASE_URL`, while rejecting hosts outside the configured allowlist before
downloading.
In `@docker-compose.yaml`:
- Around line 75-76: Replace the hardcoded postgres username and password in
every DATABASE_URL definition with Docker Compose environment-variable
substitution, using consistent variables that support deployment-time overrides
and preserve the existing host, port, and database name.
In `@pytest.ini`:
- Around line 1-2: Update the pytest configuration alongside the existing
python_files setting to explicitly add the repository root and tests directory
via pythonpath = . tests, ensuring imports such as app.config and conftest
resolve regardless of invocation directory or test path.
In `@tests/kafka_events/create/create_discussion_multi_statement_array.json`:
- Around line 26-32: Add at least one valid entry to data.solutions in
tests/kafka_events/create/create_discussion_multi_statement_array.json lines
26-32, tests/kafka_events/create/create_discussion_multi_theme_llm.json lines
26-30, and tests/kafka_events/create/create_discussion_multi_theme_local.json
lines 26-30. Keep each fixture’s existing challenges and classification scenario
unchanged so all three events pass ingestion and reach their intended
classification paths.
In `@tests/kafka_events/create/create_story_multi_barrier_single_theme.json`:
- Around line 24-32: Update the create-story fixture’s event data so it
satisfies STORY_KAFKA_SCHEMA: provide non-empty challenges, actionSteps, impact,
duration, blurb, and content values, and replace null pdfUrls with populated
original and masked URLs. Provide a valid transcriptLink as required by the
schema, preserving the existing objective and single-theme scenario.
In `@tests/unit_testing.py`:
- Around line 1317-1336: Remove the initial _fetch_story_content call before
monkeypatching and delete the unused fake_download_that_fails helper. Keep the
_download_file MagicMock patch in place before the single remaining
_fetch_story_content call so the test remains network-free and validates the
fields fallback.
- Line 1742: Remove the unreachable conditional from the assertion and directly
validate that resp.json()["status"] equals "pending", preserving the existing
response-status check.
---
Nitpick comments:
In `@app/temporal/deface_blur_activity.py`:
- Around line 38-39: Update the _blur_semaphore module-global annotation to use
Optional[asyncio.Semaphore], and add Optional to the existing typing imports so
its initial None sentinel is type-correct.
- Around line 108-113: Update the activity flow around _run_in_image_executor
and _download_file to capture the downloader’s returned local path and use it
for subsequent processing and finally cleanup. Remove the independently
recomputed DOWNLOADS_DIR / filename path, while preserving the existing
output_path behavior.
In `@docker-compose.yaml`:
- Around line 69-154: Extract the identical env_file, environment, extra_hosts,
and depends_on configuration into a shared YAML anchor or x- extension field,
then reuse it in analytics-web, analytics-consumer, analytics-worker, and
analytics-all. Preserve each service’s existing profile, ports, volumes, and
command or entrypoint settings while ensuring the shared configuration is
defined only once.
- Around line 69-154: Add appropriate Docker Compose healthchecks to
analytics-web, analytics-consumer, analytics-worker, and analytics-all so each
service reports whether its application process is responsive or healthy. Use
the existing service-specific commands, endpoints, or modes to define checks
that work inside the containers, while preserving their current dependencies,
ports, volumes, and startup commands.
In `@tests/conftest.py`:
- Around line 265-292: The module-level _test_outcomes map is incomplete in
pytest-xdist workers, causing each worker to overwrite the CSV with partial
results. Update pytest_sessionfinish to detect session.config.workerinput and
return immediately for worker processes, leaving CSV synchronization to the
controller while preserving the existing outcome aggregation.
- Around line 295-325: Update pytest_sessionfinish to run CSV synchronization
only when the designated environment flag is enabled, while preserving the
existing _test_outcomes and _CSV_PATH checks. Guard the header.index lookups for
“Test ID” and “Status” so malformed or renamed columns return without rewriting
or masking test results, and replace the direct CSV write with an atomic
temporary-file write followed by replacement of _CSV_PATH. Add the required os
import.
In `@tests/csv_uploads/not_a_csv.txt`:
- Around line 1-2: Remove the unused tests/csv_uploads/not_a_csv.txt fixture, or
update test_upload_006_non_csv_extension_rejected in tests/unit_testing.py to
load it via _csv_file("not_a_csv.txt") instead of constructing the payload
inline; choose one approach and ensure the test continues validating rejection
of a non-CSV extension.
In `@tests/unit_testing.py`:
- Around line 755-765: Update test_db_002_insert_story_stores_scalar_text and
the similar tests around the referenced locations to use the existing
_find_execute_call helper instead of indexing conn.execute.call_args_list[-1].
Extract objective_arg from the helper’s targeted call while preserving the
current assertion.
- Around line 1971-1981: The test
test_upload_023_concurrent_process_calls_race_safe currently relies on brittle
inspect.getsource text matching. Replace it with a FakeConn-based invocation of
try_claim_for_processing, capture the SQL passed to fetchval or execute, and
assert that the executed statement is a single UPDATE compare-and-swap
containing the status predicate and RETURNING status.
- Around line 431-456: Make the retry backoff used by IngestionConsumer
configurable in app/kafka/consumer.py, using a module-level constant or existing
settings symbol at the sleep calculation. In
test_kafka_020_db_failure_retries_3_times_then_dlq and the corresponding
test_kafka_021 and test_kafka_023 flows, override that value with near-zero
delays before invoking _run_consumer_briefly, then reduce the test duration
accordingly while preserving the existing retry-count and DLQ assertions.
- Around line 613-620: Replace each broad pytest.raises(Exception) assertion in
the affected tests, including the cases around
pii_and_abusive_language_detection_activity, with the specific exception type
raised by that code path, such as ValueError or json.JSONDecodeError. Verify
each expected type from the implementation and preserve the existing log
assertions so patching errors cannot satisfy the tests accidentally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cbfcbd1f-776c-4f8c-9c20-140662e9ede2
⛔ Files ignored due to path filters (8)
tests/TEST_CASES.csvis excluded by!**/*.csv,!**/*.csvtests/csv_uploads/empty.csvis excluded by!**/*.csv,!**/*.csvtests/csv_uploads/extra_columns.csvis excluded by!**/*.csv,!**/*.csvtests/csv_uploads/malformed.csvis excluded by!**/*.csv,!**/*.csvtests/csv_uploads/missing_columns.csvis excluded by!**/*.csv,!**/*.csvtests/csv_uploads/missing_session_id_value.csvis excluded by!**/*.csv,!**/*.csvtests/csv_uploads/valid_discussion.csvis excluded by!**/*.csv,!**/*.csvtests/csv_uploads/valid_story.csvis excluded by!**/*.csv,!**/*.csv
📒 Files selected for processing (20)
.env.exampleapp/config.pyapp/database/db.pyapp/services/classifier.pyapp/temporal/csv_processing_activity.pyapp/temporal/deface_blur_activity.pyapp/temporal/pii_and_abusive_activity.pyapp/temporal/worker.pydocker-compose.yamlpytest.inirequirements.txttests/conftest.pytests/csv_uploads/not_a_csv.txttests/kafka_events/create/create_discussion_multi_statement_array.jsontests/kafka_events/create/create_discussion_multi_theme_llm.jsontests/kafka_events/create/create_discussion_multi_theme_local.jsontests/kafka_events/create/create_story_multi_barrier_single_theme.jsontests/test_kafka_events.pytests/test_mode_logic.pytests/unit_testing.py
💤 Files with no reviewable changes (2)
- tests/test_mode_logic.py
- tests/test_kafka_events.py
Vivek-M-08
changed the title
- config.py: reject DATABASE_POOL_MIN_SIZE > DATABASE_POOL_MAX_SIZE at settings-load time instead of failing later with a generic asyncpg error - deface_blur_activity.py: validate resolved image host against MEDIA_BASE_URL before downloading (SSRF), and check BaseException instead of Exception in the gather() results loop so CancelledError propagates correctly instead of raising a confusing TypeError - docker-compose.yaml: parameterize the hardcoded postgres:postgres credentials via env var substitution (defaults unchanged) - pytest.ini: add explicit pythonpath so imports don't depend on invocation directory/style - fix 4 kafka event fixtures (3 discussion + 1 story) that were missing required ingestion fields (pdfUrls, transcriptLink, solutions/challenges, participantsData) and would have been DLQ'd before reaching the classification scenarios they were meant to test - tests/unit_testing.py: remove a real network call firing before its mock patch in test_rating_002, and a dead if-False conditional in test_upload_004
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/temporal/deface_blur_activity.py (1)
147-153: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake the GCS object key tenant- and image-unique.
blob_nameuses onlyblob_prefixand the final two URL path segments. Different submissions can produce the same object key. A later upload can replace another tenant's blurred image and return the wrong image URL.Include tenant identity, submission identity, and
i, or use a collision-resistant derived key.Proposed fix
- blob_name = f"{blob_prefix}/{actual_name}" + blob_name = f"{blob_prefix}/{tenant_code}/{submission_id}/{i}_{actual_name}"As per path instructions,
app/temporal/**requires idempotent activities.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/temporal/deface_blur_activity.py` around lines 147 - 153, Update the blob_name construction in the deface/blur activity to include tenant identity, submission identity, and the image index i, or another collision-resistant derived key, while preserving the existing prefix and upload flow. Ensure the resulting object key is deterministic so the activity remains idempotent and cannot collide across tenants, submissions, or images.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/temporal/deface_blur_activity.py`:
- Around line 147-153: Update the blob_name construction in the deface/blur
activity to include tenant identity, submission identity, and the image index i,
or another collision-resistant derived key, while preserving the existing prefix
and upload flow. Ensure the resulting object key is deterministic so the
activity remains idempotent and cannot collide across tenants, submissions, or
images.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59d13ff1-3818-47ce-a864-c9eba7b59577
📒 Files selected for processing (9)
app/config.pyapp/temporal/deface_blur_activity.pydocker-compose.yamlpytest.initests/kafka_events/create/create_discussion_multi_statement_array.jsontests/kafka_events/create/create_discussion_multi_theme_llm.jsontests/kafka_events/create/create_discussion_multi_theme_local.jsontests/kafka_events/create/create_story_multi_barrier_single_theme.jsontests/unit_testing.py
🚧 Files skipped from review as they are similar to previous changes (6)
- pytest.ini
- tests/kafka_events/create/create_story_multi_barrier_single_theme.json
- tests/kafka_events/create/create_discussion_multi_theme_llm.json
- docker-compose.yaml
- tests/kafka_events/create/create_discussion_multi_theme_local.json
- tests/unit_testing.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests